Send in the correct and expected types #499
Conversation
WalkthroughUpdates to utils adjust type handling: stringify response.url in JSON decode errors and instantiate GitRepoManager with a root_directory keyword argument using a string. Repository API changes rename the GitRepoManager constructor parameter from directory to root_directory. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
90802de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://653a41aa.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-correct-parameter-types.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #499 +/- ##
===========================================
- Coverage 76.25% 75.74% -0.52%
===========================================
Files 100 100
Lines 9036 8846 -190
Branches 1732 1732
===========================================
- Hits 6890 6700 -190
Misses 1670 1670
Partials 476 476
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
infrahub_sdk/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
infrahub_sdk/utils.py (2)
infrahub_sdk/exceptions.py (1)
JsonDecodeError(13-20)infrahub_sdk/repository.py (1)
GitRepoManager(9-33)
🔇 Additional comments (1)
infrahub_sdk/utils.py (1)
95-99: Good fix: ensure JsonDecodeError.url is a string as expected by the exception typeJsonDecodeError.url is annotated as str | None (see infrahub_sdk/exceptions.py), so passing str(response.url) is correct and avoids leaking an httpx.URL instance.
| repo = GitRepoManager(root_directory=str(directory)) | ||
| return str(repo.active_branch) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: returning "None" string when no active branch is present
repo.active_branch is annotated as str | None (see infrahub_sdk/repository.py). Casting it with str(...) will convert None to the literal string "None", which is misleading and violates the function’s contract to return the actual branch name. Handle the None case explicitly.
Apply this diff to avoid returning "None" and fail fast with a clear error:
- repo = GitRepoManager(root_directory=str(directory))
- return str(repo.active_branch)
+ repo = GitRepoManager(root_directory=str(directory))
+ active_branch = repo.active_branch
+ if active_branch is None:
+ raise ValueError(f"Could not determine active branch for repository at {directory!s}")
+ return active_branchRun this to find other call sites that may still use the old GitRepoManager constructor or rely on get_branch returning a non-empty string:
🏁 Script executed:
#!/bin/bash
# Find GitRepoManager instantiations and check argument usage
rg -n -C2 '\bGitRepoManager\s*\('
# Look for any usage expecting get_branch() to never be empty/None
rg -n -C2 '\bget_branch\s*\('Length of output: 5976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- infrahub_sdk/utils.py (1-400) ----"
sed -n '1,400p' infrahub_sdk/utils.py || true
echo
echo "---- infrahub_sdk/repository.py (1-240) ----"
sed -n '1,240p' infrahub_sdk/repository.py || true
echo
echo "---- infrahub_sdk/operation.py (1-160) ----"
sed -n '1,160p' infrahub_sdk/operation.py || true
echo
echo "---- infrahub_sdk/checks.py (1-240) ----"
sed -n '1,240p' infrahub_sdk/checks.py || true
echo
echo "---- infrahub_sdk/node/node.py (1-140) ----"
sed -n '1,140p' infrahub_sdk/node/node.py || true
echo
echo "---- infrahub_sdk/store.py (1-320) ----"
sed -n '1,320p' infrahub_sdk/store.py || true
echo
echo "---- Grep for active_branch usages ----"
rg -n --hidden -S 'active_branch' || true
echo
echo "---- Grep for str(...active_branch...) usages ----"
rg -n --hidden -S "str\([^)]+active_branch[^\)]*\)" || trueLength of output: 39725
Fail fast on missing git active branch — avoid returning the literal "None"
Casting repo.active_branch with str(...) will convert None to the string "None". repository.active_branch is annotated as str | None (infrahub_sdk/repository.py), and I found three call sites that currently do this conversion — they must explicitly handle None and fail fast with a clear error.
Files to change:
- infrahub_sdk/utils.py — get_branch (around lines 230–236)
- infrahub_sdk/operation.py — branch_name property (around line 42)
- infrahub_sdk/checks.py — branch_name property (around line 143)
Suggested diffs:
infrahub_sdk/utils.py
- repo = GitRepoManager(root_directory=str(directory))
- return str(repo.active_branch)
+ repo = GitRepoManager(root_directory=str(directory))
+ active_branch = repo.active_branch
+ if active_branch is None:
+ raise ValueError(f"Could not determine active branch for repository at {directory!s}")
+ return active_branchinfrahub_sdk/operation.py
- if not hasattr(self, "git") or not self.git:
- self.git = GitRepoManager(self.root_directory)
-
- self.branch = str(self.git.active_branch)
+ if not hasattr(self, "git") or not self.git:
+ self.git = GitRepoManager(self.root_directory)
+
+ active_branch = self.git.active_branch
+ if active_branch is None:
+ raise ValueError(f"Could not determine active branch for repository at {self.root_directory!s}")
+ self.branch = active_branchinfrahub_sdk/checks.py
- if not self.git:
- self.git = GitRepoManager(self.root_directory)
-
- self.branch = str(self.git.active_branch)
- return self.branch
+ if not self.git:
+ self.git = GitRepoManager(self.root_directory)
+
+ active_branch = self.git.active_branch
+ if active_branch is None:
+ raise ValueError(f"Could not determine active branch for repository at {self.root_directory!s}")
+ self.branch = active_branch
+ return self.branchReasoning: explicit None checks avoid returning the misleading string "None" and produce a clear, early error when the active branch cannot be determined. After these changes, update any callers that expect get_branch()/branch_name to always succeed or handle the ValueError appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| repo = GitRepoManager(root_directory=str(directory)) | |
| return str(repo.active_branch) | |
| repo = GitRepoManager(root_directory=str(directory)) | |
| active_branch = repo.active_branch | |
| if active_branch is None: | |
| raise ValueError(f"Could not determine active branch for repository at {directory!s}") | |
| return active_branch |
🤖 Prompt for AI Agents
infrahub_sdk/utils.py around lines 230–236, infrahub_sdk/operation.py around
line 42, and infrahub_sdk/checks.py around line 143: currently code casts
repository.active_branch to str which turns None into the literal "None";
instead, check if repo.active_branch is None and if so raise a clear ValueError
(e.g. "cannot determine active git branch") so callers fail fast, otherwise
return the branch string; update the three locations to perform this explicit
None check and raise the ValueError rather than returning str(None).
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor